Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware)

crates/papyrus_storage/src/serialization/serializers.rs line 986 at r1 (raw file):

            )?,
            abi: String::deserialize_from(&mut decompress_from_reader(bytes)?.as_slice())?,
            contract_class_version: String::deserialize_from(bytes)?,

Seems like you need to keep the order here

Code quote:

    fn deserialize_from(bytes: &mut impl std::io::Read) -> Option<Self> {
        Some(Self {
            sierra_program: Vec::<Felt>::deserialize_from(
                &mut decompress_from_reader(bytes)?.as_slice(),
            )?,
            entry_points_by_type: HashMap::<EntryPointType, Vec<EntryPoint>>::deserialize_from(
                bytes,
            )?,
            abi: String::deserialize_from(&mut decompress_from_reader(bytes)?.as_slice())?,
            contract_class_version: String::deserialize_from(bytes)?,

crates/starknet_api/src/state.rs line 218 at r1 (raw file):

    pub entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>,
    pub abi: String,
    pub contract_class_version: String,

To be consistent with the compiler's struct

Suggestion:

    pub sierra_program: Vec<Felt>,
    pub contract_class_version: String,
    pub entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>,
    pub abi: String,

crates/papyrus_storage/src/utils_test.rs line 34 at r1 (raw file):

            ClassHash(i_felt),
            SierraContractClass {
                contract_class_version: "".to_string(),

Let it be "0.1.0"

Code quote:

"".to_string(),

Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @Yoni-Starkware)

crates/papyrus_storage/src/serialization/serializers.rs line 986 at r1 (raw file):

the order of the fields?

Are you removing the duplicated object in a separate PR?

Reviewed all commit messages.
Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware)

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @Yoni-Starkware)

crates/papyrus_storage/src/utils_test.rs line 34 at r1 (raw file):

Done.

crates/starknet_api/src/state.rs line 218 at r1 (raw file):

Done.

Reviewed all commit messages.
Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @AvivYossef-starkware)

crates/papyrus_storage/src/utils.rs line 26 at r2 (raw file):

#[derive(Serialize)]
struct DumpDeclaredClass {

Can you ask @DvirYo-starkware what is this class and why it's missing the string ABI?

Code quote:

#[derive(Serialize)]
struct DumpDeclaredClass {

crates/papyrus_storage/src/utils.rs line 31 at r2 (raw file):

    compiled_class_hash: CompiledClassHash,
    sierra_program: Vec<Felt>,
    entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>,

Keep the order consistent

Suggestion:

struct DumpDeclaredClass {
    class_hash: ClassHash,
    compiled_class_hash: CompiledClassHash,
    sierra_program: Vec<Felt>,
    contract_class_version: String,
    entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>,

crates/papyrus_storage/src/utils.rs line 82 at r2 (raw file):

                            compiled_class_hash: *compiled_class_hash,
                            sierra_program: contract_class.sierra_program.clone(),
                            entry_points_by_type: contract_class.entry_points_by_type.clone(),

Suggestion:

                        &DumpDeclaredClass {
                            class_hash: *class_hash,
                            compiled_class_hash: *compiled_class_hash,
                            sierra_program: contract_class.sierra_program.clone(),
                            contract_class_version: contract_class.contract_class_version.clone(),
                            entry_points_by_type: contract_class.entry_points_by_type.clone(),

Reviewable status: 0 of 7 files reviewed, 5 unresolved discussions (waiting on @AvivYossef-starkware)

crates/papyrus_storage/src/serialization/serializers.rs line 986 at r1 (raw file):

Yes

crates/starknet_client/src/reader/objects/state.rs line 71 at r2 (raw file):

            contract_class_version: class.contract_class_version,
            sierra_program: class.sierra_program,
            entry_points_by_type: class.entry_points_by_type,

Suggestion:

        Self {
            sierra_program: class.sierra_program,
            contract_class_version: class.contract_class_version,
            entry_points_by_type: class.entry_points_by_type,

crates/papyrus_test_utils/src/lib.rs line 503 at r2 (raw file):

        pub entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>,
        pub abi: String,
        pub contract_class_version: String,

Suggestion:

    pub struct SierraContractClass {
        pub sierra_program: Vec<Felt>,
        pub contract_class_version: String,
        pub entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>,
        pub abi: String,

Reviewable status: 0 of 7 files reviewed, 5 unresolved discussions (waiting on @Yoni-Starkware)

crates/papyrus_storage/src/utils.rs line 31 at r2 (raw file):

Done.

crates/papyrus_storage/src/utils.rs line 82 at r2 (raw file):

                            compiled_class_hash: *compiled_class_hash,
                            sierra_program: contract_class.sierra_program.clone(),
                            entry_points_by_type: contract_class.entry_points_by_type.clone(),

Done.

crates/papyrus_test_utils/src/lib.rs line 503 at r2 (raw file):

        pub entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>,
        pub abi: String,
        pub contract_class_version: String,

Done.

crates/starknet_client/src/reader/objects/state.rs line 71 at r2 (raw file):

            contract_class_version: class.contract_class_version,
            sierra_program: class.sierra_program,
            entry_points_by_type: class.entry_points_by_type,

Done.

Don't forget to delete RpcContractClass

Reviewed 1 of 6 files at r1, 2 of 4 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)

crates/papyrus_storage/src/utils.rs line 26 at r2 (raw file):

Non blocking

Reviewed 1 of 6 files at r1, 3 of 4 files at r2, 3 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)

crates/papyrus_storage/src/utils.rs line 26 at r2 (raw file):

The struct DumpDeclaredClass can be deleted. I will delete it in a different PR.

crates/papyrus_storage/src/serialization/serializers.rs line 972 at r3 (raw file):

    fn serialize_into(&self, res: &mut impl std::io::Write) -> Result<(), StorageSerdeError> {
        serialize_and_compress(&self.sierra_program)?.serialize_into(res)?;
        self.contract_class_version.serialize_into(res)?;

This is a breaking change to the storage. We should discuss this.
A few months ago, we decided to break the storage in version 13.3. I don't know what was agreed upon about the storage in 13.3 and 13.4, but it is fine if we break the storage in 13.4.

crates/papyrus_protobuf/src/converters/class.rs line 209 at r3 (raw file):

        let sierra_program =
            value.program.into_iter().map(Felt::try_from).collect::<Result<Vec<_>, _>>()?;

Make sure with @ShahakShama it is fine (I don't think should be a problem here)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

crates/papyrus_storage/src/serialization/serializers.rs line 972 at r3 (raw file):

I think that we initialize papyrus with each new version, right? @Yoni-Starkware

Just two more PRS to reach it.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DvirYo-starkware)

crates/papyrus_storage/src/serialization/serializers.rs line 972 at r3 (raw file):

@AvivYossef-starkware not necessarily but we would have to do this for this version since we need all the Sierra classes. So LGTM

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware and @DvirYo-starkware)

crates/papyrus_protobuf/src/converters/class.rs line 209 at r3 (raw file):

Now the conversion to protobuf isn't the inverse of the conversion from protobuf. Fix the other conversion (I'll put a comment there

crates/papyrus_protobuf/src/converters/class.rs line 292 at r3 (raw file):

        });

        let contract_class_version = format!(

Remove this, and extract the contract_class_version from value instead

Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @DvirYo-starkware, @ShahakShama, and @Yoni-Starkware)

crates/papyrus_protobuf/src/converters/class.rs line 292 at r3 (raw file):

Thanks, consider adding a test to enforce this.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)

crates/papyrus_protobuf/src/converters/class.rs line 292 at r3 (raw file):

There's a TODO to add that test (FYI @noamsp-starkware )

Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)